Stabilize compaction E2E tests#1314
Merged
Merged
Conversation
Re-enable the compaction E2E coverage in Node, Python, and Go by matching the deterministic .NET/Rust flow: register event waiters before triggering compaction, wait for a successful compaction completion, and verify the post-compaction continuation path. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR stabilizes and re-enables compaction E2E coverage for Node.js, Python, and Go, aligning them with existing .NET/Rust behavior around deterministic compaction triggering and successful completion events.
Changes:
- Removes skips from compaction E2E tests.
- Registers event waiters before triggering compaction.
- Verifies successful compaction metadata and post-compaction session continuity.
Show a summary per file
| File | Description |
|---|---|
python/e2e/test_compaction_e2e.py |
Re-enables Python compaction E2E and waits for start/successful complete events. |
nodejs/test/e2e/compaction.e2e.test.ts |
Re-enables Node.js compaction E2E and adds successful completion assertions. |
go/internal/e2e/compaction_e2e_test.go |
Re-enables Go compaction E2E and adds channel-based event waiting/assertions. |
Copilot's findings
Comments suppressed due to low confidence (2)
nodejs/test/e2e/compaction.e2e.test.ts:65
- The stabilized .NET and Rust compaction E2Es compare these summary tags case-insensitively (dotnet/test/E2E/CompactionE2ETests.cs:56-58, rust/tests/e2e/compaction.rs:84-87). Keeping these checks case-sensitive makes this test more brittle if a valid summary varies tag casing.
const summary = completeEvent.data.summaryContent ?? "";
expect(summary).toContain("<overview>");
expect(summary).toContain("<history>");
expect(summary).toContain("<checkpoint_title>");
go/internal/e2e/compaction_e2e_test.go:117
- The stabilized .NET and Rust compaction E2Es compare these summary tags case-insensitively (dotnet/test/E2E/CompactionE2ETests.cs:56-58, rust/tests/e2e/compaction.rs:84-87). Keeping the Go checks case-sensitive makes this test more brittle if a valid summary varies tag casing.
if !strings.Contains(summary, "<overview>") {
t.Errorf("Expected summary to contain <overview>, got: %q", summary)
}
if !strings.Contains(summary, "<history>") {
t.Errorf("Expected summary to contain <history>, got: %q", summary)
- Files reviewed: 3/3 changed files
- Comments generated: 3
Add explicit compaction event timeouts in the Node test, surface session errors while waiting in the Go test, and make summary tag checks case-insensitive across SDKs. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
Cross-SDK Consistency Review ✅This PR re-enables the compaction E2E tests in Node.js, Python, and Go using the same deterministic pattern already established in the .NET and Rust reference implementations. All 5 SDKs are now consistent on:
No cross-SDK consistency issues found.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Compaction E2E coverage was still skipped in Node, Python, and Go even though the equivalent .NET and Rust tests had already been stabilized. This re-enables those skipped tests by using the same deterministic trigger flow across SDKs.
Summary
session.compaction_startand a successfulsession.compaction_complete, matching the .NET/Rust behavior that ignores transient failed compaction attempts.Validation
npx tsc --noEmitnpx vitest run --no-coverage test\e2e\compaction.e2e.test.tspytest e2e\test_compaction_e2e.py -vgo test .\internal\e2e\ -run TestCompactionE2E -v -timeout 300sThe required Opus 4.7 xhigh code-review/fix loop was run before this PR; it identified the need to wait for successful compaction completion events, which is included here.